No description provided.

No description provided.

No description provided.

Now I have everything I need for a complete review of both open PRs.

PR #129 — feat(mcp): list_rules tool

File coverage: crates/plumb-mcp/src/lib.rs, crates/plumb-mcp/tests/mcp_protocol.rs, crates/plumb-cli/tests/mcp_stdio.rs, docs/src/mcp.md

1. Determinism

list_rules_payload sorts the registry by rule.id() string before building the response. Since ids are unique <category>/<name> strings, sort_unstable_by produces a stable result — no tie-breaking ambiguity. No SystemTime, no HashMap iteration in the output path. Clean.

2. Workspace layering

No new cross-layer dependencies. plumb-mcp → plumb-core (register_builtin) is the existing allowed path. No unsafe, no println!. #![forbid(unsafe_code)] and #![deny(clippy::unwrap_used, clippy::expect_used)] already cover the crate.

3. Error handling

list_rules_payload is infallible — returns (String, Value). The async list_rules wraps it and returns Ok(...). Correct; no false Result wrapper needed. No missing # Errors section.

4. Tests

• mcp_protocol.rs::list_rules_returns_every_builtin_rule_sorted — verifies count, array length, sort order, and field shapes in-process. ✓
• mcp_stdio.rs::mcp_list_rules_returns_every_rule — full e2e over real stdio. ✓
• mcp_initialize_and_tools_list updated with presence assertions for both explain_rule and list_rules. ✓

5. Documentation

Module-level doc updated. list_rules_payload has a #[must_use] and a thorough doc block explaining the sort key, token-budget bound, and the &self design choice. docs/src/mcp.md table row added.

Punch list

• crates/plumb-mcp/src/lib.rs — #[allow(clippy::unused_self)] on a pub method is unusual. The doc comment justifies it (ergonomic symmetry with other async tool methods whose &self is load-bearing). Acceptable; the workspace pedantic group promotes unused_self to a warn, so the allow is necessary. Not a blocker.

Verdict: APPROVE

PR #132 — feat(mcp): get_config tool

File coverage: crates/plumb-mcp/src/lib.rs, crates/plumb-mcp/Cargo.toml, Cargo.lock, crates/plumb-cli/tests/mcp_stdio.rs, docs/src/mcp.md

1. Determinism

SystemTime is stored as a cache key for mtime comparison, never serialized into the response. std::collections::HashMap is used for the internal cache; its iteration order never leaks into output. clippy.toml only bans SystemTime::now() and Instant::now() — Metadata::modified() is not in the disallowed list. No output-visible nondeterminism. Clean on the letter of the rule.

The serialized response fields (config, source, path) are built from serde_json::json!({}) in insertion order. Deterministic.

2. Workspace layering

plumb-config (layer 4) added as a dep of plumb-mcp (layer 5). Not a cycle; not an upward dependency. However dependency-hierarchy.md still lists plumb-mcp as depending on only plumb-core and plumb-format — the doc is already stale (plumb-cdp was added earlier without updating it). The rule document should be updated to reflect both plumb-cdp and plumb-config. Not a hard blocker but the ADR reference is wrong.

3. Error handling

# Errors section present on get_config. Empty and relative-path guards return invalid_params. Stat and parse failures return internal_error. ConfigError is mapped through map_config_error helper. No unwrap/expect outside tests.

4. Tests — BLOCKER

crates/plumb-mcp/tests/mcp_protocol.rs has no new test for get_config. The rule in .agents/rules/mcp-tool-patterns.md is explicit:

Every tool gets a case in crates/plumb-mcp/tests/mcp_protocol.rs: Happy path. Invalid args → clear JSON-RPC error.

The PR adds only an e2e test in mcp_stdio.rs (happy path — no plumb.toml). Missing:

• An in-process protocol test for the happy path (no-file → default config shape).
• A test for working_dir = "" → invalid_params error.
• A test for a relative path → invalid_params error.

5. Additional warnings

crates/plumb-mcp/src/lib.rs ~L237-244 — TOCTOU window:

if config_path.exists() {
    let mtime = std::fs::metadata(&config_path)   // file may be gone here
        .and_then(|m| m.modified())

Between exists() and metadata() the file can be deleted, producing a confusing internal_error instead of a graceful fallback to default. Prefer: stat first, match on io::ErrorKind::NotFound.

crates/plumb-mcp/src/lib.rs — blocking I/O in async context:
config_path.exists(), std::fs::metadata(), and plumb_config::load() are all synchronous. Inside async fn get_config, they block the Tokio thread. For a local config file this rarely matters in practice, but the correct pattern is tokio::task::spawn_blocking. Non-blocking for now; flag for a follow-up if the server ever handles concurrent requests.

Unbounded cache: config_cache: Arc<Mutex<HashMap<PathBuf, ConfigCacheEntry>>> has no eviction. Not a problem at current usage but worth a tracking issue if the server becomes long-lived.

Punch list

Verdict: REQUEST_CHANGES

Conflict resolution attempt blocked. The PR conflicts with main because PR #130 (explain_rule tool) and PR #131 (lint_url over real http(s) URLs via ChromiumDriver) both landed and overlap in crates/plumb-mcp/src/lib.rs and crates/plumb-mcp/tests/mcp_protocol.rs. The intended resolution is straightforward (rebase feat(mcp): list_rules tool onto current main, then re-add the four pieces in lib.rs — module-doc bullet, ListRulesArgs struct, list_rules + list_rules_payload methods, instructions string update, dispatch arm, tool descriptor — plus the list_rules_returns_every_builtin_rule_sorted test in mcp_protocol.rs). The automated session could not write crates/plumb-mcp/src/lib.rs because the worktree harness denied write permission to that path for both root and subagent contexts (delegation-guard hook routes Rust src to a subagent; harness then refuses the subagent's writes). The rebase was aborted; the branch is back at 3fa0ea6 with a clean tree.

Conflict resolution complete. Merged origin/main into the list_rules branch — main had landed PRs #130 (explain_rule), #131 (real http(s) URLs in lint_url), and #133 (delegation-guard merge bypass). Mechanical resolution in crates/plumb-mcp/src/lib.rs and tests/mcp_protocol.rs: union both new tools (4 total — echo, lint_url, explain_rule, list_rules), switch register_builtin import to the top-level re-export main introduced, and add the list_rules row to docs/src/mcp.md.

Also addressed the prior Claude review's REQUEST_CHANGES: added mcp_list_rules_returns_every_rule end-to-end test in crates/plumb-cli/tests/mcp_stdio.rs, extended mcp_initialize_and_tools_list to assert both list_rules and explain_rule are advertised, and dropped the duplicate register_builtin() call in the in-process test.

CI: all 13 checks green (Determinism, Preflight, Tests on ubuntu/macos/windows, Coverage, MSRV, cargo-deny, Docs, Size guard, Conventional Commits, Claude review, GitGuardian, action-semantic-pull-request). Latest Claude review verdict for #129 is APPROVE; the trailing REQUEST_CHANGES in the combined verdict comment is scoped to PR #132 (get_config), unrelated to this branch.